Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filesystem abstraction #227

Merged
merged 36 commits into from
Jan 21, 2025
Merged

Conversation

Masterchef365
Copy link
Contributor

Here is a rough implementation of an abstraction over the file system and other OS functions (ex. user directories). There is a trait FileSystem, a trait object of which can be passed to FileDialog as a way of overriding the default behavior. As a way of testing the API, I threw together a browser demo which allows one to upload a ZIP file and browse its contents: https://masterchef365.github.io/file-dialog-on-web/

More to come, as I work out the rest of the implementation details. Mostly wanted to get some eyes on the API choices I've made here (is Arc the right interface?).

@fluxxcode
Copy link
Owner

This already looks very good! Thanks for working on this.

Yep, we have to use Arc to keep the file dialog thread safe.

@Masterchef365 Masterchef365 marked this pull request as ready for review December 23, 2024 00:02
@Masterchef365
Copy link
Contributor Author

Marked as ready for review, BUT I think at the very least the doc comments on the (newly exposed to the user) MetaData, Disk, Disks, and UserDirectories structs probably need updating.

Copy link
Owner

@fluxxcode fluxxcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done an initial review but haven't had time to test the changes yet. The review mainly contains name changes. Looking good overall so far!

src/data/disks.rs Outdated Show resolved Hide resolved
src/data/disks.rs Outdated Show resolved Hide resolved
src/data/disks.rs Outdated Show resolved Hide resolved
src/data/user_directories.rs Outdated Show resolved Hide resolved
src/file_dialog.rs Outdated Show resolved Hide resolved
src/create_directory_dialog.rs Outdated Show resolved Hide resolved
src/create_directory_dialog.rs Outdated Show resolved Hide resolved
src/virtual_fs.rs Outdated Show resolved Hide resolved
src/virtual_fs.rs Outdated Show resolved Hide resolved
@Masterchef365 Masterchef365 changed the title WIP: Filesystem abstraction Filesystem abstraction Dec 24, 2024
src/data/user_directories.rs Outdated Show resolved Hide resolved
src/file_dialog.rs Outdated Show resolved Hide resolved
src/virtual_fs.rs Outdated Show resolved Hide resolved
@fluxxcode
Copy link
Owner

fluxxcode commented Jan 4, 2025

@Masterchef365 Could you also add a file_system builder method to FileDialog? As with all other config values, so that you can use a FileDialog without explicitly creating a config object:

FileDialog::new().file_system(Arc::new(MyFileSystem));

The method then simply changes the file system in the config. Ty! :)

@fluxxcode
Copy link
Owner

Hey @Masterchef365, I finally found the time to test all the changes. Looking good so far! Could you fix the merge conflicts so I can merge this PR? There are also Clippy and Rustfmt bugs that need to be fixed. Ty!

@Masterchef365
Copy link
Contributor Author

Rebased and force-pushed 👍

@fluxxcode
Copy link
Owner

The CI is still failing. Could you take a look at that?

@Masterchef365
Copy link
Contributor Author

The CI is still failing. Could you take a look at that?

Looks like it's just some clippy lints. While many of them are fine, I think some are false-positives like making the constructors const. What do you think? If you want em resolved I'll resolve them, but in my personal opinion clippy shouldn't gate PRs for this reason.

@fluxxcode
Copy link
Owner

It actually makes sense for public APIs to be set to const. So that the objects can also be created for const variables, which someone might want to do.

So yup, the clippy errors should all be fixed. A few months ago we made some extra changes to Clippy to ensure good code quality. Therefore Clippy is stricter than the default settings.

Copy link
Owner

@fluxxcode fluxxcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for working on this! Will merge the PR now.

@fluxxcode fluxxcode merged commit 54fd300 into fluxxcode:develop Jan 21, 2025
5 checks passed
@fluxxcode fluxxcode mentioned this pull request Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants